Expose collection attributes from `Inspect` trait #1914

dastansam · 2023-10-17T14:17:08Z

Description

What does this PR do?

While working with pallet_nfts through nonfungibles_v2 traits Inspect, Mutate, I found out that once you have set the collection attribute with <Nfts as Mutate>::set_collection_attribute(), it's not possible to read it with <Nfts as Inspect>::collection_attribute() since they use different namespace values. When setting the attribute, AttributeNamespace::Pallet is used, while AttributeNamespace::CollectionOwner is used when reading.

more context: freeverseio/laos#7 (comment)

This PR makes item an optional parameter in Inspect::system_attribute(), to be able to read collection attributes.

Why are these changes needed?

To be able to read collection level attributes when reading attributes of the collection. It will be possible to read collection attributes by passing None for item

How were these changes implemented and what do they affect?

NftsApi is also affected and NftsApi::system_attribute() now accepts optional item parameter.

Breaking change

Because of the change in the NftsApi::system_attribute() method's item param, parachains who integrated the NftsApi need to update their API code and frontend integrations accordingly. AssetHubs are unaffected since the NftsApi wasn't released on those parachains yet.

dastansam · 2023-10-17T14:20:20Z

hey @jsidorenko

as we discussed earlier, I am submitting this PR. Do you think I should add tests for this? I don't see any tests for trait implementations, so just want to confirm.

substrate/frame/support/src/traits/tokens/nonfungible_v2.rs

jsidorenko · 2023-10-17T15:01:35Z

In regards to tests, here is the way to test your changes:

use frame_support::{traits::{tokens::nonfungibles_v2::{Create, Destroy, Inspect, Mutate}}};

assert_ok!(
	<Nfts as Mutate<<Test as SystemConfig>::AccountId, ItemConfig>>::set_collection_attribute(
		&0,
		&[1],
		&[2],
	)
);
assert_eq!(attributes(0), vec![(None, AttributeNamespace::Pallet, bvec![1], bvec![2])]);
assert_eq!(
	<Nfts as Inspect<<Test as SystemConfig>::AccountId>>::system_attribute(&0, None, &[1]),
	Some(vec![2])
);

dastansam · 2023-10-24T15:48:56Z

hey @jsidorenko

I added tests and I think it's ready for review

jsidorenko

Nice!

* master: Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Expose collection attributes from `Inspect` trait (#1914) `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897) [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023) Sort the benchmarks before listing them (#2026) publish pallet-root-testing (#2017) Contracts: Add benchmarks to include files (#2022) Small optimisation to `--profile dev` wasm builds (#1851) basic-authorship: Improve time recording and logging (#2010) Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815) [ci] Run check-rust-feature-propagation in pr and master (#2012) Improve features dev-ex (#1831) Remove obsolete comment. (#2008)

* tsv-disabling: Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Expose collection attributes from `Inspect` trait (#1914) `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897) [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023) Sort the benchmarks before listing them (#2026) publish pallet-root-testing (#2017) Contracts: Add benchmarks to include files (#2022) Small optimisation to `--profile dev` wasm builds (#1851) basic-authorship: Improve time recording and logging (#2010) Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815) [ci] Run check-rust-feature-propagation in pr and master (#2012) Improve features dev-ex (#1831) Remove obsolete comment. (#2008)

* tsv-disabling: (36 commits) Removed TODO from test-case for hard-coded delivery fee estimation (#2042) Expose collection attributes from `Inspect` trait (#1914) `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897) [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023) Sort the benchmarks before listing them (#2026) publish pallet-root-testing (#2017) Contracts: Add benchmarks to include files (#2022) Small optimisation to `--profile dev` wasm builds (#1851) basic-authorship: Improve time recording and logging (#2010) Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815) [ci] Run check-rust-feature-propagation in pr and master (#2012) Improve features dev-ex (#1831) Remove obsolete comment. (#2008) Refactor candidates test in paras_inherent (#2004) PVF: Add worker check during tests and benches (#1771) Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997) polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002) Make `IdentityInfo` generic in `pallet-identity` (#1661) Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900) `CheckWeight`: Add more logging (#1996) ...

# Description - What does this PR do? While working with `pallet_nfts` through `nonfungibles_v2` traits `Inspect, Mutate`, I found out that once you have set the collection attribute with `<Nfts as Mutate>::set_collection_attribute()`, it's not possible to read it with `<Nfts as Inspect>::collection_attribute()` since they use different `namespace` values. When setting the attribute, `AttributeNamespace::Pallet` is used, while `AttributeNamespace::CollectionOwner` is used when reading. more context: freeverseio/laos#7 (comment) This PR makes `item` an optional parameter in `Inspect::system_attribute()`, to be able to read collection attributes. - Why are these changes needed? To be able to read collection level attributes when reading attributes of the collection. It will be possible to read collection attributes by passing `None` for `item` - How were these changes implemented and what do they affect? `NftsApi` is also affected and `NftsApi::system_attribute()` now accepts optional `item` parameter. ## Breaking change Because of the change in the `NftsApi::system_attribute()` method's `item` param, parachains who integrated the `NftsApi` need to update their API code and frontend integrations accordingly. AssetHubs are unaffected since the NftsApi wasn't released on those parachains yet.

Polkadot-Forum · 2023-12-06T22:53:15Z

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/polkadot-release-analysis-v1-4-0/5152/1

# Description - What does this PR do? While working with `pallet_nfts` through `nonfungibles_v2` traits `Inspect, Mutate`, I found out that once you have set the collection attribute with `<Nfts as Mutate>::set_collection_attribute()`, it's not possible to read it with `<Nfts as Inspect>::collection_attribute()` since they use different `namespace` values. When setting the attribute, `AttributeNamespace::Pallet` is used, while `AttributeNamespace::CollectionOwner` is used when reading. more context: freeverseio/laos#7 (comment) This PR makes `item` an optional parameter in `Inspect::system_attribute()`, to be able to read collection attributes. - Why are these changes needed? To be able to read collection level attributes when reading attributes of the collection. It will be possible to read collection attributes by passing `None` for `item` - How were these changes implemented and what do they affect? `NftsApi` is also affected and `NftsApi::system_attribute()` now accepts optional `item` parameter. ## Breaking change Because of the change in the `NftsApi::system_attribute()` method's `item` param, parachains who integrated the `NftsApi` need to update their API code and frontend integrations accordingly. AssetHubs are unaffected since the NftsApi wasn't released on those parachains yet.

dastansam added 2 commits October 13, 2023 17:06

Consistent collection attribute namespace

af1ea67

System attribute collection level

e2dfa29

Update comments

fe31bb1

dastansam changed the title ~~Ensure collection attributes can be read through traits~~ [draft] Expose collection attributes from Inspect trait Oct 17, 2023

jsidorenko reviewed Oct 17, 2023

View reviewed changes

substrate/frame/support/src/traits/tokens/nonfungible_v2.rs Outdated Show resolved Hide resolved

jsidorenko reviewed Oct 17, 2023

View reviewed changes

substrate/frame/support/src/traits/tokens/nonfungible_v2.rs Outdated Show resolved Hide resolved

jsidorenko reviewed Oct 17, 2023

View reviewed changes

substrate/frame/support/src/traits/tokens/nonfungible_v2.rs Outdated Show resolved Hide resolved

jsidorenko added T2-pallets This PR/Issue is related to a particular pallet. T4-runtime_API This PR/Issue is related to runtime APIs. labels Oct 17, 2023

jsidorenko self-assigned this Oct 17, 2023

jsidorenko added the R1-breaking_change This PR introduces a breaking change and should be highlighted in the upcoming release. label Oct 17, 2023

dastansam and others added 3 commits October 23, 2023 14:09

Revert nonfungible_v2 changes

d052336

More reverts

15a5254

Merge branch 'master' into master

5a419bc

jsidorenko mentioned this pull request Oct 24, 2023

add nft apis to kusama + polkadot asset hub runtimes #1448

Closed

1 task

dastansam and others added 5 commits October 24, 2023 18:32

Add tests

7774e76

Merge branch 'master' into master

149a753

Extract collection_id in tests

7ed2129

Merge branch 'master' of github-dastansam:dastansam/polkadot-sdk

1899f79

Fix import

77420ff

dastansam changed the title ~~[draft] Expose collection attributes from Inspect trait~~ Expose collection attributes from Inspect trait Oct 24, 2023

dastansam marked this pull request as ready for review October 24, 2023 15:47

dastansam requested review from a team October 24, 2023 15:47

jsidorenko approved these changes Oct 25, 2023

View reviewed changes

Merge branch 'master' into master

7c18091

seadanda approved these changes Oct 25, 2023

View reviewed changes

Merge branch 'master' into master

858d43e

gilescope approved these changes Oct 25, 2023

View reviewed changes

liamaharon approved these changes Oct 26, 2023

View reviewed changes

Merge branch 'master' into master

e450049

jsidorenko enabled auto-merge (squash) October 26, 2023 09:50

jsidorenko merged commit 0bcebac into paritytech:master Oct 26, 2023
109 of 110 checks passed

github-actions bot mentioned this pull request Feb 19, 2024

Update substrate/polkadot/cumulus from v1.3.0 to v1.6.0 moondance-labs/tanssi#419

Closed

github-actions bot mentioned this pull request Mar 13, 2024

Update polkadot-sdk from v1.3.0 to v1.7.2 moonbeam-foundation/moonbeam#2703

Closed

Aideepakchaudhary mentioned this pull request Oct 11, 2024

Feat: Polkadot v1.2. to v1.4 upgrade Cerebellum-Network/blockchain-node#437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose collection attributes from `Inspect` trait #1914

Expose collection attributes from `Inspect` trait #1914

dastansam commented Oct 17, 2023 •

edited by jsidorenko

Loading

dastansam commented Oct 17, 2023

jsidorenko commented Oct 17, 2023 •

edited

Loading

dastansam commented Oct 24, 2023

jsidorenko left a comment

Polkadot-Forum commented Dec 6, 2023

Expose collection attributes from Inspect trait #1914

Expose collection attributes from Inspect trait #1914

Conversation

dastansam commented Oct 17, 2023 • edited by jsidorenko Loading

Description

Breaking change

dastansam commented Oct 17, 2023

jsidorenko commented Oct 17, 2023 • edited Loading

dastansam commented Oct 24, 2023

jsidorenko left a comment

Choose a reason for hiding this comment

Polkadot-Forum commented Dec 6, 2023

Expose collection attributes from `Inspect` trait #1914

Expose collection attributes from `Inspect` trait #1914

dastansam commented Oct 17, 2023 •

edited by jsidorenko

Loading

jsidorenko commented Oct 17, 2023 •

edited

Loading